Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update tests for replaceItems with actOnDispatch #752

Conversation

jd1378
Copy link
Contributor

@jd1378 jd1378 commented Jun 10, 2024

Summary

Tests for #751

@fratzinger
Copy link
Collaborator

Wow. Thanks for the fast delivery! 💚 Looks good. The only see I wonder about is 'NOTE THIS TEST NOTE THIS TEST' in one of the tests? Is this a leftover or intentional?

@jd1378
Copy link
Contributor Author

jd1378 commented Jun 10, 2024

well I just copied the test block above it and modified it for dispatch, but It looks like an important test so it might be intentional (?). but I did not add that part for sure

@fratzinger fratzinger merged commit fc9f5e4 into feathersjs-ecosystem:master Jun 10, 2024
7 checks passed
@fratzinger
Copy link
Collaborator

lol. merged it then. I want to work on a few things in this repo anyways. I don't like the implicit magic that happens around replaceItems. When to use context.data, when to use context.result depending on context.type? It does not work for around hooks at all? What do you want to achieve with replaceItems in a before hook if there already is context.result because you want to skip the adapter call.

Too many questions and to much magic for my taste. I want to introduce a more explicit replaceData and replaceResult and maybe replaceDispatch or get that covered by replaceResult(..., { useDispatch: true | false | 'ifExistent' }) or something. Just thinking out loud.

That said I'll eventually rework this stuff anyways.

@jd1378
Copy link
Contributor Author

jd1378 commented Jun 10, 2024

hmm, I found the bug for replaceItems when I was using feathers-casl. it was using replaceItems but things were not getting replaced properly* (feathers-pinia had a bug where it couldn't handle arrays, it's fixed now). so It took me some time to pin point it to the dispatch issue. and then I had to patch feathers-casl to replace both result and dispatch.
I have done A LOT of modifications to feathers-casl to get it working the way I want it to, and it's still not what I expect.
I know it's not the place to discuss it, just wanted to mention where this little utility was having impact on my project.
(btw feathers-casl's most interesting feature for me was the getChannelsWithReadAbility function, which worked very good)

@jd1378 jd1378 deleted the add_tests_for_replace_items_dispatch branch June 10, 2024 12:34
@fratzinger
Copy link
Collaborator

Wrote you on discord. I would love to chat about this stuff. Not now but in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants